fix(core): methodsOf filters out getters and symbols #2983
Conversation
WalkthroughThis change adds unit tests and modifies the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Func as methodsOf()
Test->>Func: Call methodsOf(TestClass)
Func->>Func: Iterate over class properties
alt Property is "constructor" / is a symbol / has getter
Func-->>Func: Skip property
else Valid method property
Func-->>Func: Add property to results
end
Func->>Test: Return list of method names
Assessment against linked issues
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/utils/objects/methodsOf.spec.ts(1 hunks)packages/core/src/utils/objects/methodsOf.ts(2 hunks)
🧰 Additional context used
🪛 ESLint
packages/core/src/utils/objects/methodsOf.spec.ts
[error] 1-3: Run autofix to sort these imports!
(simple-import-sort/imports)
🪛 GitHub Check: lint (20.12.2)
packages/core/src/utils/objects/methodsOf.spec.ts
[failure] 1-1:
Run autofix to sort these imports!
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-third-parties (20.12.2)
- GitHub Check: test-security (20.12.2)
- GitHub Check: test-platform (20.12.2)
- GitHub Check: test-orm (20.12.2)
- GitHub Check: test-graphql (20.12.2)
- GitHub Check: test-core (20.12.2)
- GitHub Check: test-specs (20.12.2)
🔇 Additional comments (3)
packages/core/src/utils/objects/methodsOf.spec.ts (2)
5-18: Well-structured test case for basic method retrieval!The test case effectively verifies that
methodsOfcorrectly identifies and returns only method names from a class, excluding regular properties.
19-33: Excellent test coverage for injected properties!The test case thoroughly verifies that properties decorated with
@Inject()are correctly excluded from the method list, which is crucial for the fix.packages/core/src/utils/objects/methodsOf.ts (1)
4-4: LGTM!The new import is correctly added and used in the implementation.
| import {isSymbol, isSymbolOrSymbolClass} from "./isSymbol.js"; | ||
| import {methodsOf} from "./methodsOf.js"; | ||
| import {Inject} from "@tsed/di"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Sort imports according to the project's conventions.
The static analysis tools indicate that imports need to be sorted.
Apply this diff to sort the imports:
-import {isSymbol, isSymbolOrSymbolClass} from "./isSymbol.js";
-import {methodsOf} from "./methodsOf.js";
-import {Inject} from "@tsed/di";
+import {Inject} from "@tsed/di";
+import {isSymbol, isSymbolOrSymbolClass} from "./isSymbol.js";
+import {methodsOf} from "./methodsOf.js";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import {isSymbol, isSymbolOrSymbolClass} from "./isSymbol.js"; | |
| import {methodsOf} from "./methodsOf.js"; | |
| import {Inject} from "@tsed/di"; | |
| import {Inject} from "@tsed/di"; | |
| import {isSymbol, isSymbolOrSymbolClass} from "./isSymbol.js"; | |
| import {methodsOf} from "./methodsOf.js"; |
🧰 Tools
🪛 ESLint
[error] 1-3: Run autofix to sort these imports!
(simple-import-sort/imports)
🪛 GitHub Check: lint (20.12.2)
[failure] 1-1:
Run autofix to sort these imports!
| if (isSymbol(propertyKey) || propertyKey === "constructor" || Object.getOwnPropertyDescriptor(prototypeOf(target), propertyKey)?.get) | ||
| return; | ||
| methods.set(propertyKey, {target, propertyKey}); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider improving readability of the filtering condition.
While the implementation is correct, the condition could be more readable by splitting it into separate checks with descriptive variable names.
Consider this refactoring for improved readability:
- if (isSymbol(propertyKey) || propertyKey === "constructor" || Object.getOwnPropertyDescriptor(prototypeOf(target), propertyKey)?.get)
- return;
- methods.set(propertyKey, {target, propertyKey});
+ const descriptor = Object.getOwnPropertyDescriptor(prototypeOf(target), propertyKey);
+ const isGetter = descriptor?.get !== undefined;
+ const isConstructor = propertyKey === "constructor";
+
+ if (isSymbol(propertyKey) || isConstructor || isGetter) {
+ return; // Skip symbols, constructor, and getter properties
+ }
+
+ methods.set(propertyKey, {target, propertyKey});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isSymbol(propertyKey) || propertyKey === "constructor" || Object.getOwnPropertyDescriptor(prototypeOf(target), propertyKey)?.get) | |
| return; | |
| methods.set(propertyKey, {target, propertyKey}); | |
| const descriptor = Object.getOwnPropertyDescriptor(prototypeOf(target), propertyKey); | |
| const isGetter = descriptor?.get !== undefined; | |
| const isConstructor = propertyKey === "constructor"; | |
| if (isSymbol(propertyKey) || isConstructor || isGetter) { | |
| return; // Skip symbols, constructor, and getter properties | |
| } | |
| methods.set(propertyKey, {target, propertyKey}); |
LonguCodes
force-pushed
the
fix-methods-of-inject-getter-filterout
branch
from
dfb10ec to
3407640
Compare
LonguCodes
force-pushed
the
fix-methods-of-inject-getter-filterout
branch
from
3407640 to
ba03b92
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/utils/objects/methodsOf.spec.ts(1 hunks)packages/core/src/utils/objects/methodsOf.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-third-parties (20.12.2)
- GitHub Check: test-specs (20.12.2)
- GitHub Check: test-security (20.12.2)
- GitHub Check: test-platform (20.12.2)
- GitHub Check: test-orm (20.12.2)
- GitHub Check: test-core (20.12.2)
- GitHub Check: test-graphql (20.12.2)
🔇 Additional comments (3)
packages/core/src/utils/objects/methodsOf.spec.ts (1)
3-16: LGTM! Good test coverage for basic method detection.The test effectively verifies that
methodsOfcorrectly identifies methods while excluding regular properties.packages/core/src/utils/objects/methodsOf.ts (2)
4-4: LGTM! Clean import.
19-21: Verify the filtering implementation.The filtering logic looks correct but let's verify the implementation handles all edge cases.
Let's check how this implementation is used across the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check usage patterns of methodsOf and potential edge cases # Search for methodsOf usage echo "=== methodsOf usage ===" rg "methodsOf\(" -A 3 # Search for classes with Symbol methods echo -e "\n=== Classes with Symbol methods ===" ast-grep --pattern 'class $_ { [$_]() { $$$ } }' # Search for classes with getters echo -e "\n=== Classes with getters ===" ast-grep --pattern 'class $_ { get $_() { $$$ } }'Length of output: 3451
The filtering logic in
methodsOf.tshas been verified and handles all intended edge cases.
- The implementation correctly filters out symbol properties, class constructors, and properties with getters.
- Usage across the codebase (e.g., in decorators and tests) confirms that only valid methods are retained.
| it("should not return getters", () => { | ||
| class TestClass { | ||
| method() {} | ||
|
|
||
| get property() { | ||
| return ""; | ||
| } | ||
| } | ||
|
|
||
| const methods = methodsOf(TestClass); | ||
|
|
||
| expect(methods).toHaveLength(1); | ||
| expect(methods[0]).toMatchObject({ | ||
| propertyKey: "method" | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding more test cases for comprehensive coverage.
While the getter test is good, consider adding tests for:
- Symbol methods
- Constructor method
- Multiple methods in a class
- Inherited methods from parent classes
Here's a suggested test case for symbols and inheritance:
it("should handle symbols and inheritance", () => {
const symbolMethod = Symbol("symbolMethod");
class ParentClass {
parentMethod() {}
}
class TestClass extends ParentClass {
[symbolMethod]() {}
method() {}
}
const methods = methodsOf(TestClass);
expect(methods).toHaveLength(2);
expect(methods.map(m => m.propertyKey)).toEqual(["method", "parentMethod"]);
});
|
🎉 This PR is included in version 8.4.7 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Information
fixes #2982
Todos
Summary by CodeRabbit
New Features
Tests
Refactor